[POS Settings] Adjust settings Controller-Service interface and dependencies #16053

iamgabrielma · 2025-08-28T12:07:43Z

Closes WOOMOB-1176
Closes WOOMOB-1188
Closes WOOMOB-1205

Description

This PR adjusts and cleans up the responsibilities of the PointOfSaleSettingsController by:

DI settingsService and pluginsService into the settingsController
Remove usage of siteID from the protocol
Make a dedicated struct for receipt data
Make a dedicated view model for the store details view

I've also added a temporary "ghost table" for the loading state:

Screen.Recording.2025-08-28.at.20.43.46.mov

Testing

CI should pass.
Access to POS Settings should work the same as before, there is no user-face changes except the loading state.

dangermattic · 2025-08-28T12:08:27Z

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2025-08-28T12:17:04Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16053-3e9979f`
	Version	`23.1`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`3e9979f`
	Installation URL	1n9j4tivpvesg

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

iamgabrielma · 2025-08-28T13:49:13Z

WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsStoreDetailView.swift

 }

+// Temporary: Simplified copy from PointOfSaleOrderListView.GhostOrderRowView
+private struct GhostSettingRowView: View {


This one is a mere copy of existing GhostOrderRowView, where I removed one of the extra lines. I'll iterate on this when I do the final design iteration tomorrow/next week.

just wondering, this is a separate UI enhancement from the architecture changes right? or does this become necessary after the refactoring? I'm not sure what the differences between these two structs, would be great to reuse the shared component if applicable.

this is a separate UI enhancement from the architecture changes right? or does this become necessary after the refactoring?

That's separate from the architecture changes yes, just added it temporarily to reflect isLoading state changes in the UI. I'll be updating the struct in the next PR with the rest of UI updates 👍

jaclync

Thank you for the changes and unit tests, still working as expected just some non-blocking comments.

jaclync · 2025-08-29T04:49:27Z

WooCommerce/Classes/POS/Presentation/Settings/StoreSettingsViewModel.swift

+import enum Yosemite.Plugin
+import struct Yosemite.SiteSetting
+
+final class StoreSettingsViewModel: ObservableObject {


nit: on the first glance, I thought this is a class from store management. Maybe the prefix could be added for consistency?

Yeah! Updated 037c47d

jaclync · 2025-08-29T04:52:03Z

WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsView.swift

+                                               viewModel: StoreSettingsViewModel(siteID: settingsController.siteID,
+                                                                                 settingsService: settingsController.settingsService,
+                                                                                 pluginsService: settingsController.pluginsService))


nit: with the current implementation, the view model isn't owned by any class and just initialized whenever this view is redrawn. Do we want the settings controller to own this view model, like if we want to keep the already retrieved receipt settings? This way, these 3 dependenceis (siteID, settingsService, pluginsService) don't have to be public either.

jaclync · 2025-08-29T04:54:26Z

WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsStoreDetailView.swift

 struct PointOfSaleSettingsStoreDetailView: View {
+    @State private var isLoading: Bool = false
+
    let settingsController: PointOfSaleSettingsControllerProtocol


nit: it looks like settingsController is now only needed for storeName and storeAddress. Could these two values be provided by the view model, if no other settings sections need these info? so that the view only relies on the view model, leaving settings controller for keeping global states?

Very true, updated: 3af72b4

jaclync · 2025-08-29T05:11:13Z

WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsStoreDetailView.swift

 }

+// Temporary: Simplified copy from PointOfSaleOrderListView.GhostOrderRowView
+private struct GhostSettingRowView: View {


just wondering, this is a separate UI enhancement from the architecture changes right? or does this become necessary after the refactoring? I'm not sure what the differences between these two structs, would be great to reuse the shared component if applicable.

jaclync · 2025-08-29T05:19:32Z

WooCommerce/Classes/POS/Presentation/Settings/StoreSettingsViewModel.swift

+        receiptInformation = POSReceiptInformation(
+            storeName: settingValue(from: siteSettings, settingID: "woocommerce_pos_store_name"),
+            storeAddress: settingValue(from: siteSettings, settingID: "woocommerce_pos_store_address"),
+            phone: settingValue(from: siteSettings, settingID: "woocommerce_pos_store_phone"),
+            email: settingValue(from: siteSettings, settingID: "woocommerce_pos_store_email"),
+            refundReturnsPolicy: settingValue(from: siteSettings, settingID: "woocommerce_pos_refund_returns_policy")
+        )


nit: how about moving the setting value mapping to settingsService.retrievePointOfSaleSettings for these fields in Yosemite, so that the method returns POSReceiptInformation? If there are more than receipt settings in the settings from this method, POSReceiptInformation can be included in a new struct as the return type.

Yes, at the moment is only receipt info, but I can foresee we'll have more pretty soon. Moved to Yosemite and updated: 951d0ad

jaclync · 2025-08-29T05:28:17Z

WooCommerce/WooCommerceTests/POS/Models/StoreSettingsViewModelTests.swift

+        #expect(sut.shouldShowReceiptInformation == false)
+    }
+
+    @Test func retrievePOSReceiptSettings_when_no_plugin_then_shouldShowReceiptInformation_returns_false() async throws {


nice test cases for different plugin states 👍

iamgabrielma added 4 commits August 28, 2025 18:09

inject plugin service in settings controller, add unit tests

758e021

Define POSReceiptInformation

3f4d8ab

remove siteID requirement from service protocol

b424e9a

remove isLoading from protocol

8455872

iamgabrielma added 2 commits August 28, 2025 20:25

make viewmodel for store details

a16b21c

extract StoreSettingsViewModel

be71d5e

iamgabrielma changed the title ~~[POS Settings] DI PluginService in SettingsController. Clear protocol interface. Model for store details.~~ [POS Settings] Adjust settings Controller-Service interface and dependencies Aug 28, 2025

iamgabrielma commented Aug 28, 2025

View reviewed changes

iamgabrielma marked this pull request as ready for review August 28, 2025 13:50

iamgabrielma added type: task An internally driven task. feature: POS labels Aug 28, 2025

iamgabrielma added this to the 23.2 milestone Aug 28, 2025

iamgabrielma requested a review from jaclync August 28, 2025 13:51

periphery lint

9e54eae

jaclync approved these changes Aug 29, 2025

View reviewed changes

iamgabrielma added 6 commits August 31, 2025 13:02

rename classes prefix POS

037c47d

Move store's VM ownership to settings controller

3af72b4

expose store name and address through VM

091398d

move pos settings mapping to yosemite

951d0ad

update storeAddress to retrieve and present both lines

24179d6

clean up POSSettingsControllerProtocol interface

e062c05

iamgabrielma enabled auto-merge August 31, 2025 07:45

ammend missing POSReceiptInformation

3e9979f

iamgabrielma merged commit 61afa87 into trunk Aug 31, 2025
14 checks passed

iamgabrielma deleted the task/WOOMOB-1176-DI-pluginservice-in-settings-controller branch August 31, 2025 10:36

[POS Settings] Adjust settings Controller-Service interface and dependencies #16053

[POS Settings] Adjust settings Controller-Service interface and dependencies #16053

Conversation

iamgabrielma commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

dangermattic commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iamgabrielma commented Aug 28, 2025 •

edited

Loading

dangermattic commented Aug 28, 2025 •

edited

Loading

wpmobilebot commented Aug 28, 2025 •

edited

Loading